[GITHUB-6970] MultiSourceRootProvider: reduce logging verbosity #6975
Conversation
The ClassPathProvider is also invoked for files that are only temporary present. The files are only present for a short time and between the FileObject#isValid call and the FileObject#asBytes call the file is removed leading to an IOException. This situation is a race condition and happens often enough for users to be annoyed. As this is common and the scanning error is not fatal for normal operation, this should normally not be reported. Closes: apache#6970
matthiasblaesing
added
Java
|
I don't have anything against this - I'll leave on others whether it is this or #6974. |
|
@ebarboni could you give the build from this PR a spin? Direct link: https://github.com/apache/netbeans/suites/19918053727/artifacts/1179052372 |
|
Thanks @matthiasblaesing @lahodaj I assume you'd want this merged either way given the potential for a race condition here when it's enabled? Should this be mitigated in other ways, or is it always benign? Are file deletions meant to be allowed during parsing? Should this provider also be filtering something similar to https://github.com/apache/netbeans/blob/master/ide/lsp.client/src/org/netbeans/modules/lsp/client/LSPBindings.java#L183 ?? |
|
@matthiasblaesing no more popup and nothing on the log (expected as fine) |
Yes, lets go with this. Should this be mitigated in other ways, or is it always benign? Are file deletions meant to be allowed during parsing? Its all asynchronous, so it may happen. But, normally it wasn't happening all that much so far.
Yes, we probably want something like this, good catch! Thanks! |
…s supported Closes: apache#6970
|
@neilcsmith-net @lahodaj I added a commit that should implement your suggestion. |
| if (file == null) { | ||
| return false; | ||
| } | ||
| try { | ||
| return !MultiSourceRootProvider.DISABLE_MULTI_SOURCE_ROOT && | ||
| FileOwnerQuery.getOwner(file) == null && | ||
| !file.getFileSystem().isReadOnly(); | ||
| FileObject dir = file.getParent(); | ||
| File dirFile = dir != null ? FileUtil.toFile(dir) : null; | ||
| return !MultiSourceRootProvider.DISABLE_MULTI_SOURCE_ROOT | ||
| && FileOwnerQuery.getOwner(file) == null | ||
| && !file.getFileSystem().isReadOnly() | ||
| && !(dirFile != null | ||
| && dirFile.getName().startsWith("vcs-") | ||
| && dirFile.getAbsolutePath().startsWith(System.getProperty("java.io.tmpdir"))); |
There was a problem hiding this comment.
nitpick: this growing chain of seemingly unrelated conditions might look better when torn apart into multiple checks:
if (file == null || MultiSourceRootProvider.DISABLE_MULTI_SOURCE_ROOT) {
return false;
}
try {
if (fileConditionsFail)
return false,
}
File dirFile = ...
if (parentConditionsFail) {
return false;
}
return true;
|
I advise to merge as is. I think we reached consensus, that this improves the situation. I think the discussion about the styling/structuring of the if-block can happen, but I also think it is no helpful for the release process. |
The ClassPathProvider is also invoked for files that are only temporary present. The files are only present for a short time and between the FileObject#isValid call and the FileObject#asBytes call the file is removed leading to an IOException.
This situation is a race condition and happens often enough for users to be annoyed. As this is common and the scanning error is not fatal for normal operation, this should normally not be reported.
Closes: #6970